Add an API for editing a group membership's role #9114

seanh · 2024-11-23T04:52:25Z

Add an API for editing a group member's role:

PATCH /api/groups/{pubid}/members/{userid}
{"roles": ["<new_role>"]}

Testing:

Log in as devdata_admin (http://localhost:5000/login)
Create an API token (http://localhost:5000/account/developer)
Create a group (http://localhost:5000/groups/new)
Log in as a devdata_user and join the group
Use devdata_admin's API token to promote devdata_user to admin: httpx http://localhost:5000/api/groups/{pubid}/members/acct:devdata_user@localhost --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["admin"]}'
Use the "me" alias to demote yourself to a plain member: httpx http://localhost:5000/api/groups/{pubid}/members/me --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["admin"]}'
Now that you're just a plain member if you try to promote yourself again or try to change devdata_user's role you'll get 404s.
If you try to use an invalid role you'll get a validation error, for example: httpx http://localhost:5000/api/groups/{pubid}/members/me --method PATCH --headers Authorization 'Bearer {apitoken}' --json '{"roles": ["INVALID"]}'

seanh · 2024-11-23T04:55:09Z

h/presenters/group_membership_json.py

+    def asdict(self):
+        return {
+            "authority": self.membership.group.authority,
+            "userid": self.membership.user.userid,
+            "username": self.membership.user.username,
+            "display_name": self.membership.user.display_name,
+            "roles": self.membership.roles,
+        }


This is compatible with the user objects currently returned by the get-group-members API just with roles added. (It was previously thought of as a list of users, but now we're thinking of it as a list of memberships.)

seanh · 2024-11-23T04:59:57Z

h/views/api/group_members.py

+    return [
+        GroupMembershipJSONPresenter(membership).asdict()
+        for membership in context.group.memberships
+    ]


The get-group-members API now uses the new GroupMembershipJSONPresenter instead of UserJSONPresenter. It returns the same dicts as UserJSONPresenter does but with roles added. This means that this PR also adds roles to the get-group-members API responses.

seanh · 2024-11-23T05:00:17Z

h/views/api/group_members.py

@@ -28,7 +36,6 @@ def list_members(context: GroupContext, _request):
    permission=Permission.Group.MEMBER_REMOVE,
 )
 def remove_member(context: GroupMembershipContext, request):
-    """Remove a member from a group."""


Removing these pointless docstrings.

seanh · 2024-11-23T05:04:55Z

h/views/api/group_members.py

+    if not request.has_permission(
+        Permission.Group.MEMBER_EDIT,
+        EditGroupMembershipContext(
+            context.group, context.user, context.membership, new_roles
+        ),
+    ):
+        raise HTTPNotFound()


There's no permission in the @api_config() on this view. The reason is that whether or not you have permission to change a role depends on the new role that you want to change it to (only owners can change someone's role to owner or admin, admins can change someone's role to moderator or member, etc).

The new role comes from the request JSON body and isn't available until that JSON body has been parsed and validated, and that happens in the view. So we can't use Pyramid's permission thing where it calls the security policy before calling the view.

Instead we let Pyramid call the view regardless, and then the view itself constructs a context object and calls request.has_permission().

We do need the permissions logic to be implemented in the centralized permissions system because in future another view is going to have to check MEMBER_EDIT permissions as well, so we couldn't just code the logic directly in this view. Also permissions logic just belongs in the permissions system.

seanh · 2024-11-23T05:05:17Z

h/views/api/group_members.py

+    ):
+        raise HTTPNotFound()
+
+    context.membership.roles = new_roles


There's no need for a service to carry out the business logic, it's just an assignment.

seanh · 2024-11-23T05:06:55Z

h/views/api/group_members.py

+        old_roles,
+    )
+
+    return GroupMembershipJSONPresenter(context.membership).asdict()


The edit_member() and list_members() views both use GroupMembershipJSONPresenter so they both return groups in the same format.

If we ever add a get_member() view it can use the same presenter as well.

add_member() could also use it as well but that might be a breaking change (currently it returns an HTTP 204 with no body).

seanh · 2024-11-23T05:09:16Z

tests/functional/api/groups/members_test.py

+        assert res.json == [
+            {
+                "authority": membership.group.authority,
+                "userid": membership.user.userid,
+                "username": membership.user.username,
+                "display_name": membership.user.display_name,
+                "roles": membership.roles,
+            }
+            for membership in group.memberships
+        ]


Made this test assertion more specific so I can see exactly what changes I'm making to the get-group-members API (adding the roles)

seanh · 2024-11-23T05:10:02Z

tests/functional/api/groups/members_test.py

+        assert res.json == [
+            {
+                "authority": membership.group.authority,
+                "userid": membership.user.userid,
+                "username": membership.user.username,
+                "display_name": membership.user.display_name,
+                "roles": membership.roles,
+            }
+            for membership in group.memberships
+        ]


marcospri · 2024-11-27T09:05:53Z

h/security/predicates.py

+    old_roles = context.membership.roles
+    new_roles = context.new_roles
+
+    def get_authenticated_users_roles():


This and the equivalent get_authenticated_users_membership could be eventually refactored and be a helper method on identity.

Yeah that's a good idea. I'll add a card for this to Misha's onboarding project

marcospri · 2024-11-27T09:06:42Z

h/security/predicates.py

+            return True
+
+        if GroupMembershipRoles.ADMIN in authenticated_users_roles:
+            # Admins can change their own role to anything but admin.


but owner?

Yep, fixed 👍

marcospri · 2024-11-27T09:07:40Z

h/security/predicates.py

+    if not authenticated_users_roles:
+        return False
+
+    if identity.user.userid == context.user.userid:


These are all an user changing it own roles

marcospri · 2024-11-27T09:11:44Z

h/views/api/groups.py

@@ -133,16 +133,3 @@ def update(context: GroupContext, request):
    group = group_update_service.update(context.group, **appstruct)

    return GroupJSONPresenter(group, request).asdict(expand=["organization", "scopes"])
-
-
-# @TODO This is a duplication of code in h.views.api — move to a util module


6 years in the making.

seanh requested a review from marcospri November 23, 2024 04:52

seanh commented Nov 23, 2024

View reviewed changes

seanh force-pushed the add-edit-membership-api- branch from 0c02bd7 to d0791b7 Compare November 23, 2024 05:07

seanh commented Nov 23, 2024

View reviewed changes

seanh force-pushed the add-edit-membership-api- branch 2 times, most recently from 79efb82 to 702c89e Compare November 23, 2024 06:26

seanh force-pushed the add-edit-membership-api branch from a0a645d to 078fb8c Compare November 26, 2024 14:04

Base automatically changed from add-edit-membership-api to main November 26, 2024 14:28

Add an API for editing a group membership's role

b30f3cd

seanh force-pushed the add-edit-membership-api- branch from 702c89e to b30f3cd Compare November 26, 2024 14:34

marcospri approved these changes Nov 27, 2024

View reviewed changes

Correct a code comment

480b4be

seanh merged commit b64c396 into main Nov 27, 2024
9 checks passed

seanh deleted the add-edit-membership-api- branch November 27, 2024 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an API for editing a group membership's role #9114

Add an API for editing a group membership's role #9114

seanh commented Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024

seanh Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024

seanh Nov 23, 2024

marcospri Nov 27, 2024 •

edited

Loading

seanh Nov 27, 2024

marcospri Nov 27, 2024

seanh Nov 27, 2024

marcospri Nov 27, 2024

marcospri Nov 27, 2024

Add an API for editing a group membership's role #9114

Add an API for editing a group membership's role #9114

Conversation

seanh commented Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh Nov 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanh commented Nov 23, 2024 •

edited

Loading

seanh Nov 23, 2024 •

edited

Loading

marcospri Nov 27, 2024 •

edited

Loading